Needs work

@jorgebucaran Any ideas why this might be failing in CI still? According to https://docs.github.com/en/actions/reference/environment-variables, all the env values that we use to determine colour support should be set.

All my colorette tests are passing after we merged your PR, so at least that worked.

I see you are using options.enabled, are you changing its value back after using it? It's basically a singleton.

See this test.

why was this closed?

@mcollina I am still fighting with failing CI and don't want people flooded with notifications about interim commits.

@kibertoad For some reason NO_COLOR is in the environment, causing this:

let enabled =
    !isDisabled && (isForced || isWindows || isCompatibleTerminal || isCI)

...to always evaluate to false.

According to https://no-color.org:

NO_COLOR

All command-line software which outputs text with ANSI color added should check for the presence of a NO_COLOR environment variable that, when present (regardless of its value), prevents the addition of ANSI color.

Manually setting colorette.options.enabled to true wouldn't work either, because colorized = colorizer(10) has already had the ANSI codes added via colorizer().

Maybe this is tap messing with NO_COLOR. I just replaced tap with tape, and all tests passed (on GitHub).

@jorgebucaran Great finding! I think there's an option for colours in tap, I'll try that.

d767067 probably won't work. tap sets NO_COLOR to 1 to comply with Chalk's implementation of NO_COLOR (which is inconsistent with the spec).

@jorgebucaran Seems to have worked, though, now the only reason why it is failing is code coverage that dropped for some reason.

Pull Request Test Coverage Report for Build 1150442036

• 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

Totals
Change from base Build 1116256302:	0.0%
Covered Lines:	305
Relevant Lines:	305

💛 - Coveralls

@mcollina @jsumners Ready for review!

@mcollina Adjusted PR to no longer modify state, now it works the same way it did with chalk, we just do readonly check of colorette colour support resolution results.

@jsumners See https://github.com/pinojs/pino-pretty/pull/218/checks?check_run_id=3353755751

Code is good, we need to fix CI.

@jsumners See #218 (checks)

Remove the glob from the npm script and add it to the .taprc:

files:
	- '**/*.test.js'

@jsumners That seems to completely break library imports for some reason.

update: ah, wait, looks like this glob then also includes the node_modules and tries to run tests from there

Removed Windows from CI for now, because it seems to be failing for reasons unrelated to colorette migration, but glob is fine now.

Any ideas why test: test/cli-rc.test.js cli throws on invalid default config file might be flaky on MacOS? Should I disable MacOS test execution?

Any ideas why test: test/cli-rc.test.js cli throws on invalid default config file might be flaky on MacOS? Should I disable MacOS test execution?

Not a clue.

@mcollina CI is green!

@jsumners I think I should bump the major for this. WDYT?

@jsumners I think I should bump the major for this. WDYT?

Agreed.